Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Design the ROS2 implementation for Clock class #575

Merged
merged 7 commits into from
Nov 26, 2022
Merged

Design the ROS2 implementation for Clock class #575

merged 7 commits into from
Nov 26, 2022

Conversation

GiulioRomualdi
Copy link
Member

@GiulioRomualdi GiulioRomualdi commented Nov 18, 2022 •
edited
Loading

This PR develops the ROS2 implementation for the Clock class in the system component. This PR uses plain CMake instead of ament_cmake (provided by ROS2). The required ROS version is humble.

The clock can be used as follows:

  1. In c++
#include <BipedalLocomotion/System/RosClock.h>
#include <BipedalLocomotion/System/Clock.h>

#include <chrono>
#include <memory>

using namespace std::chrono_literals;

int main(int argc, char *argv[])
{
    BipedalLocomotion::System::ClockBuilder::setFactory(std::make_shared<BipedalLocomotion::System::RosClockFactory>(argc, argv));

    BipedalLocomotion::clock().sleepFor(2000ms);

    return 0;
}
  1. In python
import bipedal_locomotion_framework as blf
import sys
import datetime

# Call this function to set the ros clock
blf.system.ClockBuilder.set_factory(blf.system.RosClockFactory(args=sys.argv))

start = blf.clock().now()
blf.clock().sleep_for(datetime.timedelta(seconds=7))

@GiulioRomualdi GiulioRomualdi self-assigned this Nov 18, 2022
@traversaro
Copy link
Collaborator

@GiulioRomualdi GiulioRomualdi requested a review from traversaro

I am supposed to wait for it being not Draft.

@GiulioRomualdi
Copy link
Member Author

Actually yes, there is a problem related to

m_clock.sleep_for(...)

function.
There seems that the context is not valid.
Looking online there seems that I should call rclcpp::init() before calling the clock functions, however this method should be called only once for each ROS node. So a possibility is to check if this has been already called by someone else and if not call it

@GiulioRomualdi GiulioRomualdi force-pushed the clock/ros2 branch 2 times, most recently from 7c5dd45 to 01dd947 Compare November 22, 2022 16:27
@GiulioRomualdi GiulioRomualdi marked this pull request as ready for review November 22, 2022 16:27
@GiulioRomualdi
Copy link
Member Author

The PR is ready

S-Dafarra
S-Dafarra reviewed Nov 22, 2022
View reviewed changes
src/System/RosImplementation/src/RosClock.cpp Outdated

void RosClock::yield()
{
std::this_thread::yield();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to mix ROS2 and std calls? Is it better to simply do anything?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. Removed

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big problem, but it could make sense to have std::this_thread::yield() in this context. In the end, sleep/sleepUntil are functions that in the end are waiting on some kind of OS synchronization primitive (mutex/condition variables), so it could make sense that you want to yield even if you are waiting on those. Anyhow, I do not think it is a big problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just thought that the effect could have not been very predictable given the sleep implementation of ROS2, but maybe I am being paranoid.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In ros we should use the command spin so in theory yield is not needed. Shall we add a debug print that remember the user that this implementation of yield does nothing?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we add a debug print that remember the user that this implementation of yield does nothing?

I would avoid the print, as it would make writing generic code quite difficult (you would need to not call yield if you use the ROS2 clock). I would say let's merge it empty (safest choice), and then let's see if problems emerge.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok merged thank you all as usual ❤️

@traversaro
Copy link
Collaborator

@GiulioRomualdi GiulioRomualdi requested a review from traversaro

I am supposed to wait for it being not Draft.

Sorry, that was originally meant to be a question, I put a . just for a typo.

@GiulioRomualdi GiulioRomualdi mentioned this pull request Nov 24, 2022
Merged
@GiulioRomualdi GiulioRomualdi merged commit 2a66363 into master Nov 26, 2022
@GiulioRomualdi GiulioRomualdi deleted the clock/ros2 branch November 26, 2022 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@traversaro traversaro traversaro approved these changes

@S-Dafarra S-Dafarra S-Dafarra left review comments

Assignees

@GiulioRomualdi GiulioRomualdi

Labels
🏗️ GitHub action 🔨 component: System 🐍 bindings
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@GiulioRomualdi @traversaro @S-Dafarra